Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimized Dockerfile to reduce uncompressed image size #276

Merged
merged 2 commits into from
Aug 29, 2023

Conversation

jarylc
Copy link
Contributor

@jarylc jarylc commented Aug 5, 2023

Combining apk del commands in the same layer as their add commands shaves off ~270MB when uncompressed.

❯ docker images
REPOSITORY        TAG       IMAGE ID       CREATED          SIZE
<none>            <none>    16b82802079b   9 minutes ago    1.64GB <- new
milesmcc/shynet   latest    945cbbe7305b   8 days ago       1.91GB <- current

Combining apk del commands in the same layer as their add commands shaves off ~270MB when uncompressed.
@milesmcc
Copy link
Owner

milesmcc commented Aug 6, 2023

Thanks!

@jarylc
Copy link
Contributor Author

jarylc commented Aug 7, 2023

I'm quite curious what caused the compressed image size to go from 200-300MB to jump to 750MB from version v0.10.0 to v0.11.0 though.

When uncompressed right now, the image is taking up 1/10 of the space of my 20GB VPS. Which ultimately inspired me to do this PR, but I wonder if we can find a way to reduce the image size even more after this is merged.

EDIT 1:
Looking at the commit diffs, it seems it's after the addition of rust might be attributed to the rise in image size.

# libffi-dev and rust are used for the cryptography package,
# which we indirectly rely on. Necessary for aarch64 support.

If it's not needed in amd64, maybe we could do an architecture check during build time (using arch or uname -m) and determine if libffi-dev, rust, and cargo are required.

EDIT 2:
I tried building the initial apk installation layer like this instead:

RUN apk update && \
	apk add --no-cache gettext bash npm postgresql-libs && \
	test "$(arch)" != "x86_64" && apk add libffi-dev rust cargo || echo "amd64 build, skipping Rust installation"
	# libffi-dev and rust are used for the cryptography package,
	# which we indirectly rely on. Necessary for aarch64 support.

This yields even better sizes for amd64 systems, but unfortunately arm64 still has to take a hit unless we can remove rust as a dependency entirely.

 ❯ docker images                                                                                                                                                                                                                                                                                    [08:32:47]
REPOSITORY   TAG       IMAGE ID       CREATED          SIZE
<none>       <none>    2cda0b101cd3   10 seconds ago   744MB <- rust removed
<none>       <none>    41af2fc866fc   3 minutes ago    1.64GB <- with rust

@milesmcc @CasperVerswijvelt do let me know if you guys are okay with this approach, I can append this change to this PR as well.

@CasperVerswijvelt
Copy link
Contributor

@jarylc while I'm not really familiar with all the dependencies of Shynet, if the libffi-dev and rust are used for the cryptography package, which we indirectly rely on. Necessary for aarch64 support comment is accurate, I am all for this change.

I'm also curious why such huge dependencies are needed at all, just for the cryptography package (and why they are only needed when building for aarch64). Could you maybe shed some more light on that, @milesmcc?

@milesmcc
Copy link
Owner

milesmcc commented Aug 9, 2023

I don't entirely remember; I think the issue was that for the aarch64 build, a lot of binaries do not come pre-compiled, so we need to install the rust/libffi-dev toolchains to compile those dependencies ourselves. Perhaps the story has gotten better since then.

If aarch64 builds (and runs) correctly without rust or libffi-dev, then that suggests that precompiled binaries are now available for aarch64, in which we can drop those deps.

@jarylc
Copy link
Contributor Author

jarylc commented Aug 18, 2023

I'll try to find some time to test removing the dependencies with my RPI4 sometime soon.

@jarylc
Copy link
Contributor Author

jarylc commented Aug 22, 2023

I'm currently testing 1628820 live on my websites now (on amd64), will update EoD if any issues

EDIT: Alright, looks like all is good on amd64 without these 2 dependencies, shaving half of the image size! Hope we can get this change in first.

@milesmcc milesmcc merged commit 4cc2dd4 into milesmcc:master Aug 29, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants